Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add hasOption method to NodeElement class. #766

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

kukko
Copy link

@kukko kukko commented Oct 18, 2018

In my case that was a useful method, to check that a select element has an option or not. And I think this will be also useful for others.

@stof
Copy link
Member

stof commented Oct 18, 2018

This will trigger 2 changes of the data of the form, to check whether it has an option or no. that's quite bad as implementation of the check.

@kukko
Copy link
Author

kukko commented Oct 18, 2018

Yeah you are right with this. I will rewrite it to don't change the value of the select.

src/Element/NodeElement.php Show resolved Hide resolved
*/
public function hasOption($option){
if ('select' !== $this->getTagName()) {
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is wrong. It must return a boolean

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I writed this because if the element is not a select tag, It is not logical to return a boolean. That would be nice if I throw an exception instead of return null? Or editing the documentation of method to @return Boolean|null?

Copy link
Member

@aik099 aik099 Oct 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it's better to throw an exception.

return;
}

$optionElement = $this->find('named', array('option', $option));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can safely inline this variable, because it's used only once.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants